Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle lifetimes correctly in sym in inline and global asm #116087

Closed
wants to merge 3 commits into from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 23, 2023

Fixes #111709
Fixes #96304

sym in global-asm can never reference regions other than 'static, so it suffices to fold the constant and replace erased regions with 'static (see first commit). However, we want to be able to mention both early- and late-bound (and local, e.g. #111709) regions from the function in inline-asm.

This change is a bit invasive, but I'm not really convinced it can be simpler, at least without having to teach global_asm! how to be an inference body or something.

This conceptually reverts part of dc345d8 for inline asm, insofar as we return to treating SymFn in non-global_asm bodies as a regular HIR exprs.

cc @Amanieu who stabilized sym in #93333

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 23, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors changed the title Handle lifetimes correctly in in sym in inline and global asm Handle lifetimes correctly in sym in inline and global asm Sep 23, 2023
@@ -128,7 +128,7 @@ pub(super) fn generics_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Generics {
Node::Expr(&Expr { kind: ExprKind::InlineAsm(asm), .. })
if asm.operands.iter().any(|(op, _op_sp)| match op {
hir::InlineAsmOperand::Const { anon_const }
| hir::InlineAsmOperand::SymFn { anon_const } => {
| hir::InlineAsmOperand::SymFnInGlobal { anon_const } => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not reachable here since this is only reached for ExprKind::InlineAsm, not `ItemKind::GlobalAsm``.

@Amanieu
Copy link
Member

Amanieu commented Sep 23, 2023

Why is it necessary to change SymFnInInline to no longer use an AnonConst? Aren't inline const blocks able to properly handle lifetimes?

@compiler-errors
Copy link
Member Author

inline consts (const {}) can handle lifetimes, but sym are not represented as inline consts -- theyre AnonConst, which act somewhat differently. They're separate items, so they are type checked totally separately from their enclosing item. Inline consts act more like closures where they're type checked together with their enclosing body.

@bors
Copy link
Contributor

bors commented Oct 7, 2023

☔ The latest upstream changes (presumably #116310) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 12, 2023

inline consts (const {}) can handle lifetimes, but sym are not represented as inline consts -- theyre AnonConst, which act somewhat differently. They're separate items, so they are type checked totally separately from their enclosing item. Inline consts act more like closures where they're type checked together with their enclosing body.

Is anon const actually the desirable semantics here, or it just happens to be the current semantics?
I suspect that references in inline asm are an uncommon corner case, and we can change it if necessary.

at least without having to teach global_asm! how to be an inference body or something.

That's what I'd expect to do.
If the piece of code is an expression, that may have generic arguments and lifetimes in it (and value path is such an expression), then it should be represented as a body, no? Regardless of whether it's a global or local asm.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2023
@compiler-errors
Copy link
Member Author

If the piece of code is an expression, that may have generic arguments and lifetimes in it (and value path is such an expression), then it should be represented as a body, no? Regardless of whether it's a global or local asm.

This is not possible at the moment. Global-asm aren't type-checked in the sense of HIR bodies, nor are they represented with a HIR body.

I'd rather just altogether deny lifetime arguments in sym, if both of y'all are unconvinced by the complexity of this approach.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2023
@petrochenkov
Copy link
Contributor

This is not possible at the moment. Global-asm aren't type-checked in the sense of HIR bodies, nor are they represented with a HIR body.

What do you mean by not possible?
If e.g. enum discriminants can be bodies, then symbols in global asm certainly could as well.

I'd rather just altogether deny lifetime arguments in sym

Why it it ok to have type arguments, but not lifetime arguments?
I'm actually not sure how the path expressions in sym are type checked if they are not bodies, but they are indeed type checked at least to some extent because this fails

fn foo<T: Copy>() {}

sym foo::<NonCopy>

The question about anon consts vs inline consts also wasn't answered.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 26, 2023
@compiler-errors
Copy link
Member Author

enum discriminants can be bodies, then symbols in global asm certainly could as well.

That's because enum discriminants actually correspond to expressions in HIR.

Why it it ok to have type arguments, but not lifetime arguments?

Because we explicitly erase lifetime arguments during writeback in HIR.

I'm actually not sure how the path expressions in sym are type checked if they are not bodies, but they are indeed type checked at least to some extent because this fails

They are bodies. They just don't have a parent body, which is required for inline-consts to make sense. Inline consts are type-checked in the parent body, like closures. It's like having a closure with no enclosing body -- it doesn't make sense.

@petrochenkov: Remind me what specific question about inline you want answered, though?

@petrochenkov
Copy link
Contributor

Remind me what specific question about inline you want answered, though?

What would be the preferable semantics for sym - inline const or anon const (if backward compatibility, etc is out of question).

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 26, 2023

That's because enum discriminants actually correspond to expressions in HIR.

Okay, then sym should correspond to expressions in HIR.
SymFnInInline does correspond to an expression, SymFnInGlobal should do that too, I don't see why they should diverge.

Yes, SymFnInGlobal is an expression in a "global" context, outside of any parent body, but that's exactly the same situation as with const initializers or enum variant discriminants, if there's no top level body then we should create it.

@compiler-errors
Copy link
Member Author

Yes, SymFnInGlobal is an expression in a "global" context, outside of any parent body, [...] if there's no top level body then we should create it.

I predict this is going to be prohibitively complicated.

but that's exactly the same situation as with const initializers or enum variant discriminants

This is not exactly the same situation as const initializers or enum variant discriminants, since they never have the possibility of participating in region inference with infer lifetimes that may be inherited from the parent, nor can they reference late-bound regions (which are very very broken, even with #[feature(generic_const_expressions)], cc #115486).

Their restrictions means that it's okay for them to be represented as an AnonConst, which only allows them to reference lifetimes from the parent in certain situations, and even then, only when generic_const_exprs is enabled due to the way we set up the generics for an anon-const.

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 17, 2023

Don't care to work on this anymore. inline_asm is still super broken when lifetimes are involved, and I still maintain that it needs a more invasive fix like this, for the reasons I elaborated above.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 21, 2025
…-obk

Give `global_asm` a fake body to store typeck results, represent `sym fn` as a hir expr to fix `sym fn` operands with lifetimes

There are a few intertwined problems with `sym fn` operands in both inline and global asm macros.

Specifically, unlike other anon consts, they may evaluate to a type with free regions in them without actually having an item-level type annotation to give them a "proper" type. This is in contrast to named constants, which always have an item-level type annotation, or unnamed constants which are constrained by their position (e.g. a const arg in a turbofish, or a const array length).

Today, we infer the type of the operand by looking at the HIR typeck results; however, those results are region-erased, so during borrowck we ICE since we don't expect to encounter erased regions. We can't just fill this type with something like `'static`, since we may want to use real (free) regions:

```rust
fn foo<'a>() {
  asm!("/* ... */", sym bar::<&'a ()>);
}
```

The first idea may be to represent `sym fn` operands using *inline* consts instead of anon consts. This makes sense, since inline consts can reference regions from the parent body (like the `'a` in the example above). However, this introduces a problem with `global_asm!`, which doesn't *have* a parent body; inline consts *must* be associated with a parent body since they are not a body owner of their own. In rust-lang#116087, I attempted to fix this by using two separate `sym` operands for global and inline asm. However, this led to a lot of confusion and also some unattractive code duplication.

In this PR, I adjust the lowering of `global_asm!` so that it's lowered in a "fake" HIR body. This body contains a single expression which is `ExprKind::InlineAsm`; we don't *use* this HIR body, but it's used in typeck and borrowck so that we can properly infer and validate the the lifetimes of `sym fn` operands.

I then adjust the lowering of `sym fn` to instead be represented with a HIR expression. This is both because it's no longer necessary to represent this operand as an anon const, since it's *just* a path expression, and also more importantly to sidestep yet another ICE (rust-lang#137179), which has to do with the existing code breaking an invariant of def-id creation and anon consts. Specifically, we are not allowed to synthesize a def-id for an anon const when that anon const contains expressions with def-ids whose parent is *not* that anon const. This is somewhat related to rust-lang#130443 (comment), which is also a place in the compiler where synthesizing anon consts leads to def-id parenting issue.

As a side-effect, this consolidates the type checking for inline and global asm, so it allows us to simplify `InlineAsmCtxt` a bit. It also allows us to delete a bit of hacky code from anon const `type_of` which was there to detect `sym fn` operands specifically. This also could be generalized to support `const` asm operands with types with lifetimes in them. Since we specifically reject these consts today, I'm not going to change the representation of those consts (but they'd just be turned into inline consts).

r? oli-obk -- mostly b/c you're patient and also understand the breadth of the code that this touches, please reassign if you don't want to review this.

Fixes rust-lang#111709
Fixes rust-lang#96304
Fixes rust-lang#137179
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 21, 2025
…-obk

Give `global_asm` a fake body to store typeck results, represent `sym fn` as a hir expr to fix `sym fn` operands with lifetimes

There are a few intertwined problems with `sym fn` operands in both inline and global asm macros.

Specifically, unlike other anon consts, they may evaluate to a type with free regions in them without actually having an item-level type annotation to give them a "proper" type. This is in contrast to named constants, which always have an item-level type annotation, or unnamed constants which are constrained by their position (e.g. a const arg in a turbofish, or a const array length).

Today, we infer the type of the operand by looking at the HIR typeck results; however, those results are region-erased, so during borrowck we ICE since we don't expect to encounter erased regions. We can't just fill this type with something like `'static`, since we may want to use real (free) regions:

```rust
fn foo<'a>() {
  asm!("/* ... */", sym bar::<&'a ()>);
}
```

The first idea may be to represent `sym fn` operands using *inline* consts instead of anon consts. This makes sense, since inline consts can reference regions from the parent body (like the `'a` in the example above). However, this introduces a problem with `global_asm!`, which doesn't *have* a parent body; inline consts *must* be associated with a parent body since they are not a body owner of their own. In rust-lang#116087, I attempted to fix this by using two separate `sym` operands for global and inline asm. However, this led to a lot of confusion and also some unattractive code duplication.

In this PR, I adjust the lowering of `global_asm!` so that it's lowered in a "fake" HIR body. This body contains a single expression which is `ExprKind::InlineAsm`; we don't *use* this HIR body, but it's used in typeck and borrowck so that we can properly infer and validate the the lifetimes of `sym fn` operands.

I then adjust the lowering of `sym fn` to instead be represented with a HIR expression. This is both because it's no longer necessary to represent this operand as an anon const, since it's *just* a path expression, and also more importantly to sidestep yet another ICE (rust-lang#137179), which has to do with the existing code breaking an invariant of def-id creation and anon consts. Specifically, we are not allowed to synthesize a def-id for an anon const when that anon const contains expressions with def-ids whose parent is *not* that anon const. This is somewhat related to rust-lang#130443 (comment), which is also a place in the compiler where synthesizing anon consts leads to def-id parenting issue.

As a side-effect, this consolidates the type checking for inline and global asm, so it allows us to simplify `InlineAsmCtxt` a bit. It also allows us to delete a bit of hacky code from anon const `type_of` which was there to detect `sym fn` operands specifically. This also could be generalized to support `const` asm operands with types with lifetimes in them. Since we specifically reject these consts today, I'm not going to change the representation of those consts (but they'd just be turned into inline consts).

r? oli-obk -- mostly b/c you're patient and also understand the breadth of the code that this touches, please reassign if you don't want to review this.

Fixes rust-lang#111709
Fixes rust-lang#96304
Fixes rust-lang#137179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants